BREAKING CHANGE: ADDomain: Change Domain Install Tracking File to NetLogon Registry Test and Refactor #566
Conversation
Codecov Report
@@ Coverage Diff @@
## master #566 +/- ##
=======================================
Coverage 98.89% 98.89%
=======================================
Files 23 23
Lines 3087 3087
=======================================
Hits 3053 3053
Misses 34 34
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewed 3 of 6 files at r1, 8 of 8 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @X-Guardian)
CHANGELOG.md, line 35 at r4 (raw file):
Refactor. ([issue #560
Maybe remove this full stop after Refactor.
Btw. Some of the text you entered in the PR description would be suitable here too, it would improve the release notes. 🙂
source/ActiveDirectoryDsc.psd1, line 23 at r4 (raw file):
PowerShellVersion = '5.0'
Maybe this should be added to the change log? You think this is a breaking change? 🤔
|
Looks good to me. Just two tiny comments! Awesome work! 🙂 |
johlju
added
waiting for code fix
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johlju)
CHANGELOG.md, line 35 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Refactor. ([issue #560Maybe remove this full stop after
Refactor.Btw. Some of the text you entered in the PR description would be suitable here too, it would improve the release notes. 🙂
Done.
I also increased the markdownlint MD013 line length rule from 80 to 120 characters. (80 is too short!) and fixed all the long lines in the CHANGELOG.
source/ActiveDirectoryDsc.psd1, line 23 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
PowerShellVersion = '5.0'Maybe this should be added to the change log? You think this is a breaking change? 🤔
Good point. Done
johlju
added
needs review
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5.
Reviewable status:complete! all files reviewed, all discussions resolved
CHANGELOG.md, line 35 at r4 (raw file):
Previously, X-Guardian (Simon Heather) wrote…
Done.
I also increased the
markdownlintMD013 line length rule from 80 to 120 characters. (80 is too short!) and fixed all the long lines in theCHANGELOG.
It was 80 so that it is easier to review without having the text line-wrap. But if you want to have it at 120 that is no problem.
|
@X-Guardian Leave this so you can merge it correctly. |
johlju
added
ready for merge
X-Guardian
changed the title
X-Guardian
removed
the
ready for merge
Pull Request (PR) description
This PR changes the
ADDomainresource to use a NetLogon registry item test instead of a tracking file. TheGet-TargetResourcefunction is also refactored as follows:Get-ADDomainandGet-ADForestto uselocalhostas the server.Try/Catchblocks to only cover cmdlet calls.The resource property checks were removed from the
Test-TargetResourceas these cannot be changed once the domain has been installed.The
ADDomainunit tests were also refactored.The NewChildDomain example was also updated to clarify the contents of the credential parameter and use Windows 2016 rather than 2012 R2.
New
ADDomainIntegration tests were added.The required PowerShell version for the module was increased from v4.0 to v5.0.
This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
and comment-based help.
This change is